Skip to content

Conversation

@anthonyting
Copy link
Contributor

Problem

After this PR removing the legacy graph, there were still some components of the old graph which were not removed/replaced properly causing some bugs:

  1. aws.previewStateMachine command in command palette has an invalid translation.
  2. aws.previewStateMachine command throws an error when used from the command palette.
  3. aws.stepfunctions.openWithWorkflowStudio command (which is used for opening WFS with the context menu) throws an error when used from the command palette.
  4. Render graph option aws.renderStateMachineGraph in the AWS explorer for state machines throws an error.

Solution

  1. Replace previewStateMachine translation with openWithWorkflowStudio.
  2. Use the document URI instead of the document in the previewStateMachine command when passing to vscode.openWith.
  3. Disable aws.stepfunctions.openWithWorkflowStudio in the command palette (The aws.previewStateMachine command already works).
  4. Remove aws.renderStateMachineGraph, and just re-use previewStateMachine for the explorer nodes as well.

  • Treat all work as PUBLIC. Private feature/x branches will not be squash-merged at release time.
  • Your code changes must meet the guidelines in CONTRIBUTING.md.
  • License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

@anthonyting anthonyting requested a review from a team as a code owner February 21, 2025 01:59
@github-actions
Copy link

  • This pull request modifies code in src/* but no tests were added/updated.
    • Confirm whether tests should be added or ensure the PR description explains why tests are not required.
  • This pull request implements a feat or fix, so it must include a changelog entry (unless the fix is for an unreleased feature). Review the changelog guidelines.
    • Note: beta or "experiment" features that have active users should announce fixes in the changelog.
    • If this is not a feature or fix, use an appropriate type from the title guidelines. For example, telemetry-only changes should use the telemetry type.

@anthonyting anthonyting force-pushed the feature/stepfunctions-workflow branch from 02fdd1f to b467251 Compare February 21, 2025 02:17

const textEditor = await vscode.window.showTextDocument(doc)
await previewStateMachineCommand.execute(textEditor)
await vscode.commands.executeCommand(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may be missing something but why can't you use the previewStateMachienCommand.execute here still? You should be able to pass in the URI now

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, it may be helpful to enhance openTextDocument as needed:

export async function openTextDocument(filenameGlob: vscode.GlobPattern): Promise<vscode.TextDocument | undefined> {

instead of calling 'vscode.openWith' directly. That allows us to (potentially) instrument that or improve it in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may be missing something but why can't you use the previewStateMachienCommand.execute here still? You should be able to pass in the URI now

I initially tried to use previewStateMachineCommand.execute directly, however since I now call downloadStateMachineDefinition directly in the previewStateMachineCommand, it creates a circular dependency which fails the build.

Also, it may be helpful to enhance openTextDocument as needed:

export async function openTextDocument(filenameGlob: vscode.GlobPattern): Promise<vscode.TextDocument | undefined> {

instead of calling 'vscode.openWith' directly. That allows us to (potentially) instrument that or improve it in the future.

I think openTextDocument is not quite the interface we want to use/overload, since it seems to primarily be used for searching for a file to open, while in this case we want to open a specific URI.

I can instead refactor out vscode.openWith(uri, WorkflowStudioEditorProvider.viewType, ...) however, since we have been repeating that pattern a few times.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can instead refactor out vscode.openWith(uri, WorkflowStudioEditorProvider.viewType, ...) however, since we have been repeating that pattern a few times.

🚀

@justinmk3
Copy link
Contributor

@anthonyting
Copy link
Contributor Author

Are there assets like https://github.com/aws/aws-toolkit-vscode/blob/cc0455f112c3be8db3232acfb777341f3225c557/packages/core/resources/js/graphStateMachine.js that we can drop , as part of this PR?

Not yet, it is still in use by the CDK graph visualization command aws.cdk.renderStateMachineGraph. We will remove these assets once we upgrade aws.cdk.renderStateMachineGraph.

@jpinkney-aws jpinkney-aws merged commit 624740f into aws:feature/stepfunctions-workflow Feb 24, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants